Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom props for rows in EuiBasicTable and EuiInMemoryTable. #869

Merged
merged 6 commits into from
May 25, 2018

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented May 23, 2018

Closes #867

This adds support for a __props__ property on the item which defines a row. Alternatively we could define the row item like this:

{
  cells: { /* define each cell here */ },
  /* Add pass-through props here */
}

Though this would be a breaking change, and I think adding pass-through props will be a rarity, so I decided to just create an additional property with the underscores to avoid colliding with any of the other properties.

While I was in this code I also slightly reorganized and simplified the tests. I can extract this to a separate PR if anyone objects.

Copy link

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Was wondering if the current table columns property is sufficient for allowing custom props at the cell level? e.g if you needed to know the individual cell a user was clicking on what would be the best way of doing it?

@@ -50,7 +50,7 @@ const createUsers = (countries) => {
github: index < 10 ? github[index] : github[index - 10],
dateOfBirth: dob,
nationality: random.oneToOne(countries.map(country => country.code), index),
online: index % 2 === 0
online: index % 2 === 0,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - did you mean to add this trailing comma here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep this was deliberate. Good spot!

@cjcenizal
Copy link
Contributor Author

I'm having second thoughts on this approach. The items with which we populate a table could come from anywhere in the codebase, e.g. an external service or even a Redux store. It seems like this solution conflates concerns by requiring consumers to mix UI code with non-UI code. It also seems inefficient to make the user traverse the entire items array to decorate each item, when the table itself will already perform similar traversals.

What if the table accepted a rowProps prop?

const getRowProps = (row, id, index) => {
  const { firstName } = row;
  return {
    onClick: () => console.log(`clicked ${firstName}`),
    'aria-label': `row-${firstName}`,
    className: 'customRowClass',
  };
};

<EuiBasicTable items={items} rowProps={getRowProps} />

This is a similar pattern to what we already have with itemId. We could even duck-type this prop by letting it accept a function or an object (which might be best for ML's use case since they just want to apply a single onClick handler to all rows).

Thoughts?

@cjcenizal
Copy link
Contributor Author

Was wondering if the current table columns property is sufficient for allowing custom props at the cell level?

Great point! Yes I think this would work well. I'll see if I can add this in this PR.

@chandlerprall
Copy link
Contributor

I like the idea of a rowProps prop quite a bit. It probably makes sense to add a cellProps as well, which would resolve ML's need for correlating the cell with another element's representation of the data.

@peteharverson
Copy link

In ML, we currently only have need for the rowProps (detecting onMouseEnter and onMouseLeave for a table row, but adding cellProps too would seem to make this functionality future-proof.

@cjcenizal cjcenizal force-pushed the table-row-pass-through-props branch from 5ef8582 to bbbe378 Compare May 25, 2018 00:02
@cjcenizal
Copy link
Contributor Author

@peteharverson @chandlerprall Ready for another look. I also introduced a breaking change which applies pass-through props to the <td> element (they were originally applied to the <div> child of the <td>).

@cjcenizal
Copy link
Contributor Author

🏆 I just have to point out that all credit for this idea goes to @chandlerprall who first proposed it in #836!

Copy link

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes LGTM. I like the way this is done with rowProps and cellProps!

@@ -157,8 +158,29 @@ export function getItemId(item, props) {
}
}

export class EuiBasicTable extends Component {
function getRowProps(item, rowProps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should either add rowIndex here, or remove columnIndex from getCellProps (I lean toward removing it from the other function, not sure what value the index provides, and in theory whatever creates getRowProps/getCellProps has context for determining the index).

@cjcenizal cjcenizal force-pushed the table-row-pass-through-props branch from 7a160da to 775a0c3 Compare May 25, 2018 16:13
@cjcenizal
Copy link
Contributor Author

Good point @chandlerprall! Updated.

@cjcenizal cjcenizal force-pushed the table-row-pass-through-props branch from e345ff9 to ed982f4 Compare May 25, 2018 16:56
@cjcenizal cjcenizal merged commit f5d202f into elastic:master May 25, 2018
@cjcenizal cjcenizal deleted the table-row-pass-through-props branch May 25, 2018 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants